feat: Implement NoopAuthManager and integrate AuthManager into RestCa…#544
feat: Implement NoopAuthManager and integrate AuthManager into RestCa…#544lishuxu wants to merge 5 commits intoapache:mainfrom
Conversation
lishuxu
commented
Jan 29, 2026
lishuxu
commented
- Add NoopAuthManager for "none" authentication type
- Register "none" auth type in static registry with case-insensitive matching
- Add KnownAuthTypes() to distinguish known-but-unimplemented types (NotImplemented) from unknown types (InvalidArgument)
- Integrate AuthManager into RestCatalog
| /// \brief Fetch server configuration from the REST catalog server. | ||
| Result<CatalogConfig> FetchServerConfig( | ||
| const ResourcePaths& paths, const RestCatalogProperties& current_config, | ||
| const std::shared_ptr<auth::AuthSession>& session) { |
There was a problem hiding this comment.
| const std::shared_ptr<auth::AuthSession>& session) { | |
| const auth::AuthSession& session) { |
If the session cannot be null and it is read-only, let's use its reference directly.
There was a problem hiding this comment.
Changed the session argument type to auth::AuthSession&. Authenticate() is non-const to support OAuth2 token refresh.
|
|
||
| std::string_view RestCatalog::name() const { return name_; } | ||
|
|
||
| Result<std::unordered_map<std::string, std::string>> RestCatalog::AuthHeaders() const { |
There was a problem hiding this comment.
Is it better to add AuthSession* session as an optional input parameter to HttpClient::Get (and its friends)? Auth headers should be handled internally in the client instead of scattering them around here.
There was a problem hiding this comment.
The current design keeps HttpClient focused on HTTP transport concerns, while RestCatalog handles authentication as a business-layer responsibility. So I think current design may be better.
| [[maybe_unused]] HttpClient& shared_client, | ||
| [[maybe_unused]] const std::unordered_map<std::string, std::string>& properties) | ||
| override { | ||
| return AuthSession::MakeDefault({}); |
There was a problem hiding this comment.
Should we check auth type from properties and return error for unimplemented auth types instead of blindly returning the noop impl?
There was a problem hiding this comment.
Type checking is done in AuthManagers::Load() before manager creation. NoopAuthManager is only created for auth type "none".
2 participants
